perf(rpc): resolve block records by binary search, not by walking the chain - #87
Open
rabbitson87 wants to merge 3 commits into
Open
perf(rpc): resolve block records by binary search, not by walking the chain#87rabbitson87 wants to merge 3 commits into
rabbitson87 wants to merge 3 commits into
Conversation
… chain Context::record_for_hash walked every record in the block log to find one. The log holds one entry per applied block and nothing removes it, so at a mainnet tip that is ~963k comparisons per call -- and getblock, getblockheader and the index read path all land there. It is the same shape as the gettxoutproof scan, on a hotter path. Step 1 already knows the height, because the block tree gave it. That makes the lookup a binary search over a height-ordered log. The log is append-only in height order -- add_block pushes, and the only removal is the tail pop a disconnect performs on the applied tip -- and the codebase already relied on that: crates/node had record_at_height and record_at_height_hash doing exactly this. Rather than copy them, they move to crates/rpc beside BlockRecord and node imports them, so one implementation serves both and forty lines of duplicate go away. Context::block_by_height and Context::block_hash_at_height were scanning the same way and now use them too. Step 2 stays linear on purpose. Without the tree there is no height to search on, and a hash-keyed index would have to be maintained for every block to serve a path only legacy state reaches. The comment says so. Three tests cover the parts the rpc side never had: that a hash is matched within a duplicate-height run rather than the run's first record being assumed, that a hash absent from the run does not resolve to a sibling, and that a log which does not start at height zero still resolves. The duplicate-height test is shaped deliberately. Heights [1, 1, 2] put the dense fast path's index straight onto the second duplicate, where the height check alone would accept it and only the preceding-record guard rejects it. An earlier version used heights starting at zero, passed, and did not touch the guard at all -- the mutation audit caught that the test was not testing what its name claimed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…factor Both conflicts were additive, not semantic: the crate root export list gained entries on each side, and the block-source file gained a BlockTreeAdapter impl on main where this branch had deleted the duplicate lookup helpers. The merge resurrected those helpers alongside the import that replaced them, so they are deleted again here. The premise still holds after the refactor: record_for_hash on the new main still walks the log linearly in all four places this branch replaces. Re-ran the mutation audit rather than trusting the tests for staying green through a merge that moved the code they pin. Both mutations still turn the intended tests red. One change is worth noting: under the hash-ignoring mutation node used to fail a test too, and no longer does -- main routing that path through BlockTreeAdapter cost node its detector. The rpc side still catches it twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rabbitson87
added a commit
that referenced
this pull request
Aug 21, 2026
…ing it" This reverts 539d8da. The change duplicates #87, which was opened two days earlier and makes the same argument: `record_for_hash` has the height from the block tree, the log is ordered by height, so the lookup is a search. #87 also covers `Context::block_hash_at_height`, which 539d8da missed. What 539d8da had and #87 does not - a benchmark, the sweep-against-a-scan equivalence tests, `block_by_height_without_an_applied_tip_reads_the_log`, and five further mutations - moves to #87 rather than being dropped. This branch keeps only the chain-info fold, which nothing else covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The PR claimed a performance win with no number attached. This adds the refactor-set benchmark, both arms over one fixture in one process. 10,000 records, hash at tip: 12.17 us -> 39.6 ns 307x 100,000 records, hash at tip: 415.90 us -> 50.2 ns 8,285x 500,000 records, hash at tip: 3.737 ms -> 46.1 ns 81,092x 963,124 records, hash at tip: 7.707 ms -> 48.3 ns 159,569x 963,124 records, hash in middle: 3.328 ms -> 48.6 ns 68,528x The new arm is flat at 37-50 ns across 96x the records: a binary search is ~20 steps at a mainnet tip. Two lookup positions are measured because reporting one would flatter the scan - the tip is its worst case, the middle costs it half. The linear scan is written out in the benchmark and in the new tests rather than called through the crate: it is two lines, and an oracle that shares code with the implementation cannot disagree with it. Three tests are added on top of the three already here. Two sweep the search against that scan over every height in and around the fixture and every hash in it - a search is wrong at its boundaries, and a test that picks one pair picks whether it visits them. The third covers `block_by_height` with no applied tip, which was a gap: replacing that fallback's whole body with "the last record in the log" turned nothing red, and the new test is the only one that kills it. The shared fixture starts at height 1 rather than 0. That is load-bearing: the preceding-record guard in the direct-index fast path only matters when index `h` holds a record at height `h` that is not the first at that height, and a log starting at zero can never be in that state. Re-audited at seven mutations, all killed, baseline and restored green. Write-up in docs/benchmarks/block-record-lookup.md. Ported from perf/chaininfo-fold, where the same change was written a second time before this PR was noticed. That copy is reverted; this is the one that stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while auditing #85 and #86, and larger than either of them.
The problem
Context::record_for_hashwalked every record in the block log to find one. The log holds one entry per applied block and nothing removes it, so at a mainnet tip that is ~963k comparisons per call — andgetblock,getblockheaderand the index read path'sblock_at_heightall land there.It is the same shape as the
gettxoutproofscan that #85 removes, on a hotter path.The fix
Step 1 already knows the height, because the block tree gave it. That makes the lookup a binary search over a height-ordered log.
The log is append-only in height order —
add_blockpushes, and the only removal is the tailpopa disconnect performs on the applied tip. The codebase already relied on this:crates/node/src/block_source.rshadrecord_at_heightandrecord_at_height_hashdoing exactly this search, with tests.Rather than copy them, they move to
crates/rpcbesideBlockRecordand node imports them — one implementation for both, and 40 lines of duplicate deleted.Context::block_by_heightandContext::block_hash_at_heightwere scanning the same way and now use them too.Step 2 stays linear on purpose. Without the tree there is no height to search on, and a hash-keyed index would have to be maintained for every block to serve a path only legacy state reaches. The comment says so.
Tests
Three cover what the rpc side never had:
The duplicate-height test is shaped deliberately: heights
[1, 1, 2]put the dense fast path's index straight onto the second duplicate, where the height check alone would accept it and only the preceding-record guard rejects it.An earlier version of that test used heights starting at zero. It passed — and the mutation audit showed it passed with the guard removed, because an index past the end of a 3-record log never enters the fast path at all. The test was not testing what its name claimed. This is the shape that does.
Mutation audit
block_by_height_prefers_tree_identity_over_stale_cacheAn earlier run of this audit is not reported here because it was invalid, not because it passed: a second audit was editing the same file concurrently, and its baseline disagreed with its own restored run. Re-run serially.
Not in this PR
BlockRecordstill carries an 80-byte header the block tree also holds, and the tree itself never drops a node. Both are per-block memory costs; separate work.🤖 Generated with Claude Code
Update: the measurement this PR was missing
This PR argued for a change without a number. It has one now — and the audit was
re-run at seven mutations instead of two.
crates/rpc/benches/blocklookup.rs, both arms over one fixture in one process:before_scanafter_searchFlat at 37–50 ns across 96x the records: ~20 binary-search steps at a mainnet
tip. Two lookup positions are measured because reporting one would flatter
the scan — the tip is its worst case, the middle costs it half.
The linear scan is written out in the benchmark and in the new tests rather than
called through the crate: it is two lines, and an oracle that shares code with
the implementation cannot disagree with it.
Three more tests
Two sweep the search against that scan over every height in and around the
fixture and every hash in it — including hashes at the wrong height, which
must find nothing. A search is wrong at its boundaries, and a test that picks one
pair picks whether it visits them.
The third closes a real gap:
block_by_heightwith no applied tip wasuncovered. Replacing that fallback's whole body with "the last record in the
log" turned nothing red.
block_by_height_without_an_applied_tip_reads_the_logis the only test that kills that mutation.
The shared fixture starts at height 1 rather than 0, and that is load-bearing:
the preceding-record guard only matters when index
hholds a record at heighththat is not the first at that height, and a log starting at zero can neverbe in that state.
Re-audited
record_at_height_hashskips the rewind to the run headrecord_at_heightskips the rewind to the run headrecord_at_heighttrusts the direct index unconditionallyrecord_at_heightdrops only the preceding-record guardrecord_for_hashignores the hash and takes the run headblock_by_heightwithout a tip answers the last recordBaseline and restored: 181 passed, 0 failed.
Provenance
This change was written a second time on
perf/chaininfo-fold(#89) before thisPR was noticed. That copy is reverted there; the measurement, the tests and the
audit from it are ported here. #89 keeps only the chain-info fold.
Full write-up:
docs/benchmarks/block-record-lookup.md.